Skip to content

Conversation

jonsimantov
Copy link
Contributor

@jonsimantov jonsimantov commented Jun 13, 2025

Description

Provide details of the change, and generalize the change in the PR title above.

This PR swaps out the Analytics stub on desktop for a version that wraps the GA Windows DLL.

It's designed to work dynamically, so if it can load the DLL and verify its SHA256 hash, it uses it, otherwise it transparently operates in stub mode as always. On other desktop platforms it defaults back to stub mode.

The included DLL is itself a stub, so this change is effectively a no-op (aside from logging) at the moment.


Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

Integration tests have been manually run on Windows with and without the DLL, and confirmed to remain stubs on other desktop platforms.


Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

google-labs-jules bot and others added 27 commits June 11, 2025 23:42
refactor: Remove stale comments and TODOs from Windows Analytics

Cleans up src/analytics_desktop.cc by:
- Removing outdated TODO comments related to header verification and type definitions (Parameter/Item) that have since been resolved.
- Deleting informational comments that were no longer accurate after refactoring (e.g., comments about placeholder types, forward declarations for removed types).
- Removing an empty anonymous namespace that previously held an internal type alias.

This commit improves the readability and maintainability of the Windows Analytics implementation by removing distracting or misleading commentary. The TODO regarding the precise Future creation mechanism in stubbed functions remains, as it requires further investigation of Firebase internal APIs.
Replaces placeholder comments with actual calls to LogError() and
LogWarning() throughout the Windows Analytics implementation in
src/analytics_desktop.cc.

Key changes:
- Added #include "firebase/log.h".
- Updated error handling in LogEvent, ConvertParametersToGAParams, and
  SetUserProperty to use LogError() for invalid arguments or internal
  failures.
- Updated warnings for unexpected data types or structures in event
  parameters to use LogWarning().
- Added LogWarning() calls to all stubbed functions (e.g., SetConsent,
  InitiateOnDeviceConversionMeasurement*, SetSessionTimeoutDuration)
  to inform developers that these operations are not supported and
  have no effect on the Windows platform.

This change enhances the robustness and diagnosability of the Windows
Analytics module by providing clear feedback through the Firebase
logging system.
Based on explicit feedback, this commit fundamentally changes how
Parameter values of type Map and Vector are processed in the Windows
Analytics C++ SDK (in src/analytics_desktop.cc):

1.  **Parameter values of type Vector (`param.value.is_vector()`):**
    - These are now treated as UNSUPPORTED for top-level event parameters
      on Desktop.
    - An error is logged, and the parameter is skipped.
    - The previous logic that interpreted a vector of maps as an item array
      has been removed from ConvertParametersToGAParams.

2.  **Parameter values of type Map (`param.value.is_map()`):**
    - This case is now explicitly handled.
    - The key-value pairs of the input map are transformed into a
      GoogleAnalytics_ItemVector.
    - Each entry (key, value_variant) in your map becomes a distinct
      GoogleAnalytics_Item within this ItemVector.
    - Each such GoogleAnalytics_Item stores the original map key under a
      property named "name", and the original map value (which must be a
      primitive type) under a typed property (e.g., "int_value",
      "double_value", "string_value").
    - This resulting ItemVector is then added to the event's parameters
      using the original Parameter's name.

3.  **Comments:**
    - Code comments within ConvertParametersToGAParams have been updated
      to reflect this new processing logic.

This change aligns the behavior with specific design requirements for how
map and vector type parameters should be translated to the underlying
Windows C API for Google Analytics.
This commit incorporates detailed review feedback to align the
Windows Analytics C++ implementation (src/analytics_desktop.cc)
with common Firebase C++ SDK patterns for managing Futures and
including headers.

Key changes:

1.  **Future Management for Stubbed APIs:**
    - Replaced the previous Future creation mechanism (MakeFuture) for
      stubbed functions (GetAnalyticsInstanceId, GetSessionId, and their
      LastResult counterparts) with the standard FutureData pattern.
    - Added a static FutureData instance (g_future_data), initialized in
      firebase::analytics::Initialize() and cleaned up in Terminate().
    - Defined an internal AnalyticsFn enum for Future function tracking.
    - Stubbed Future-returning methods now use g_future_data to create
      and complete Futures.
    - As per feedback, these Futures are completed with error code 0
      and a null error message. A LogWarning is now used to inform
      developers that the called API is not supported on Desktop.
    - Added checks for g_future_data initialization in these methods.

2.  **Include Path Updates:**
    - Corrected #include paths for common Firebase headers (app.h,
      variant.h, future.h, log.h) to use full module-prefixed paths
      (e.g., "app/src/include/firebase/app.h") instead of direct
      "firebase/" paths.
    - Removed a stale include comment.

These changes improve the consistency of the Windows Analytics stubs
with the rest of the Firebase C++ SDK.
refactor: Address further review comments for Windows Analytics

This commit incorporates additional specific feedback on the Windows
Analytics C++ implementation (src/analytics_desktop.cc):

1.  **Comment Cleanup:**
    - Removed a commented-out unused constant and its description.
    - Removed a non-functional comment next to a namespace declaration.
    - Clarified and shortened the comment in the `is_vector` handling
      block within `ConvertParametersToGAParams`.

2.  **Use Common `AnalyticsFn` Enum:**
    - Included `analytics/src/common/analytics_common.h`.
    - Removed the local definition of `AnalyticsFn` enum, now relying on
      the common definition (assumed to be in
      `firebase::analytics::internal`).

3.  **Corrected Map Parameter to `GoogleAnalytics_Item` Conversion:**
    - Critically, fixed the logic in `ConvertParametersToGAParams` for when
      a `Parameter::value` is a map.
    - Previously, each key-value pair from the input map was incorrectly
      creating a `GoogleAnalytics_Item` with fixed property names like
      "name" and "int_value".
    - The logic now correctly ensures that each key-value pair from your
      input map becomes a direct property in the `GoogleAnalytics_Item`,
      using the original map's key as the key for the property in the
      `GoogleAnalytics_Item`. For example, an entry `{"user_key": 123}`
      in the input map now results in a property `{"user_key": 123}`
      within the `GoogleAnalytics_Item`.
This commit incorporates the latest round of specific feedback on the
Windows Analytics C++ implementation (src/analytics_desktop.cc):

1.  **Comment Cleanup:**
    - Removed a large commented-out block that previously contained a
      local `AnalyticsFn` enum definition.
    - Removed a comment in `Initialize()` related to marking the `app`
      parameter as unused.
    - Removed an outdated comment in `Initialize()` that described the
      function as a placeholder.
    - Removed a type comment from the `ConvertParametersToGAParams()`
      function signature.
    - Replaced a lengthy comment in `LogEvent()` regarding C API object
      lifecycle with a more concise one.

2.  **Refined Map Parameter Processing Logic:**
    - In `ConvertParametersToGAParams`, when handling a `Parameter` whose
      value is a map, the logic for creating `GoogleAnalytics_Item`
      objects from the map's entries has been clarified.
    - A local boolean flag (`successfully_set_property`) is used for each
      map entry to track if a value was successfully set in the
      corresponding `GoogleAnalytics_Item`.
    - A `GoogleAnalytics_Item` is only added to the `GoogleAnalytics_ItemVector`
      if a property was successfully set. Otherwise, the item is destroyed.
      This prevents empty or partially formed items (e.g., from map entries
      with unsupported value types) from being included in the ItemVector.
As per review feedback, this commit moves the Windows Analytics C++
implementation file from src/analytics_desktop.cc to
analytics/src/analytics_desktop.cc.

The content of the file remains the same as the previous version,
incorporating all prior review feedback.

I attempted to run the code formatter script after the move, but the
script failed due to an internal error. Formatting was not applied.
Renamed the parameter `library_path` to `library_filename` in the
`VerifyAndLoadAnalyticsLibrary` function (declaration in
`analytics_windows.h` and definition and usage in
`analytics_windows.cc`).

This change improves clarity, as the parameter is expected to be
only the DLL's filename, not a full path. The full path for
file operations is constructed internally within the function using
_wpgmptr and this filename.
If no hash is passed in, load the DLL indiscriminantly.
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Jun 13, 2025
Copy link

github-actions bot commented Jun 13, 2025

Integration test with FLAKINESS (succeeded after retry)

Requested by @a-maurice on commit c2ce64d
Last updated: Tue Aug 26 19:42 PDT 2025
View integration test log & download artifacts

Failures Configs
app_check [TEST] [FLAKINESS] [Android] [1/3 os: macos] [1/2 android_device: emulator_ftl_target]
(1 failed tests)  CRASH/TIMEOUT
messaging [TEST] [FLAKINESS] [Android] [1/3 os: windows] [1/2 android_device: android_target]
(1 failed tests)  FirebaseMessagingTest.TestSendMessageToToken
storage [TEST] [FLAKINESS] [Android] [1/3 os: windows] [1/2 android_device: emulator_ftl_target]
(1 failed tests)  CRASH/TIMEOUT

Add flaky tests to go/fpl-cpp-flake-tracker

@jonsimantov jonsimantov added the tests-requested: quick Trigger a quick set of integration tests. label Jun 25, 2025
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: succeeded This PR's integration tests succeeded. and removed tests-requested: quick Trigger a quick set of integration tests. tests: succeeded This PR's integration tests succeeded. labels Jun 25, 2025
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 25, 2025
@a-maurice
Copy link
Contributor

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a robust mechanism for using a verified Analytics DLL on Windows desktop platforms. The changes include dynamic DLL loading, SHA256 hash verification to prevent tampering, and a fallback to stub mode if the DLL is not present or invalid. The implementation is well-structured, with good error handling and security considerations like file locking during verification. I've identified one critical potential memory safety issue regarding pointer lifetimes and one opportunity for code simplification. Overall, this is a great enhancement.

@a-maurice a-maurice added tests-requested: quick Trigger a quick set of integration tests. and removed tests: succeeded This PR's integration tests succeeded. labels Aug 26, 2025
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. labels Aug 26, 2025
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 26, 2025
@a-maurice a-maurice added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Aug 26, 2025
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: succeeded This PR's integration tests succeeded. and removed tests-requested: quick Trigger a quick set of integration tests. labels Aug 26, 2025
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 26, 2025
@a-maurice a-maurice merged commit c2ce64d into main Aug 27, 2025
57 of 61 checks passed
@a-maurice a-maurice deleted the analytics-dll-secure branch August 27, 2025 01:01
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: succeeded This PR's integration tests succeeded. and removed tests: succeeded This PR's integration tests succeeded. labels Aug 27, 2025
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-release-notes Skip release notes check tests: succeeded This PR's integration tests succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants